docs+test: correct ADR-074 D2 and gate reusable-workflow boundary - #308

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

Merge Risk: 🔵 Low · up to 0b153

This PR mostly updates documentation and adds policy tests describing per-artifact SLSA Build L2 claims; it does not change runtime signing or attestation behavior. One newly added documentation test has a scope gap: it can pass even if a published verification command's ${ID} placeholder is set incorrectly elsewhere in SECURITY.md, so the guardrail intended to catch a wrong identity pin in published docs may not catch every case. This is a low-risk, self-contained issue in test coverage rather than a production or security defect, and is safe to fix before or shortly after merge.

_{Comment @coderabbitai help to get the list of available commands.}

Friendly ping — this is rebased onto current main (incl. the grpc bump), DCO/lint/make test green locally, and ready for a look when you have a moment.

Happy to address any review notes. Also still needs a trustee /ok to test so the NVIDIA runner checks can run (same gate as #295).

Thanks —
cc @lalitadithya @ndipebot @mchmarny

Rebased onto main (includes #316 failureLog). Branch still needs human review and trustee /ok to test before CI runs.

Rebased onto latest main.

Hi @lalitadithya @ndipebot @mchmarny — gentle bump when you have a moment. ADR-074 D2 / attest boundary (issue #301) is rebased onto latest main and ready for review. It still needs a trustee /ok to test so the NVIDIA runner checks can run. Happy to address any notes. Thanks!

/ok to test cd31edb

🌿 Preview your docs: https://nvidia-preview-pull-request-308.docs.buildwithfern.com/cluster-readiness-engine

Rebased onto main (includes the #325–#328 wave). Head moved cd31edb → 97cec77; another trustee /ok to test is needed for NVIDIA runners.

/ok to test 97cec77

Rebased onto main (post-#329/#331/#332). Fresh trustee /ok to test needed if the SHA moved.

ndipebot review follow-up (adr-074-d2-build-level-gate)

Pushed 2fe17c5 (tests) and 0b15384 (docs/workflows). make lint = 0; go test ./test/releasepolicy/... and make test green locally. Fresh /ok to test needed for CI on these commits.

Addressed (all 14 ndipebot threads + attest.yml header)

CodeRabbit 3961385868 was already resolved (per-workflow attest call in 60f9b93).

Deferred

• Create an actual repository ruleset over v* tags (3982022817): docs now name the control; creating it on NVIDIA/cluster-readiness-engine needs maintainer admin rights.

Addressed ndipebot's 2nd-pass (and the open CodeRabbit ${ID} note) in cf4b87c.

Still-open items from the 2nd pass

1. Bracket-index env laundering — provenance env/run scan now rejects inputs[ / needs[ as well as inputs. / needs.*.outputs.*.
2. resolvedDependencies[0].uri — executed provenance step asserts the SLSA source URI, not only digest.gitCommit.

Nits

• ADR-074 release-identity parenthetical: release.yml on v* only (dropped publish.yml).
• attest.yml header matches the ADR origin-fields + subjectKind carve-out.
• release.yml comment: ~14 near-simultaneous calls.
• ADR-074 added to gateTestCitePages.
• TestSecurityMdPinsAnExactIdentity requires the literal NVIDIA identity (no ${ID} escape hatch).

make lint 0 issues; go test ./test/releasepolicy/... ./test/docspolicy/... and make test green.

Fresh /ok to test would help re-run CI on the new head (cf4b87c).

All 7 unresolved threads (6 ndipebot + 1 CodeRabbit) replied + resolved; nothing deferred.

Third pass, on cf4b87c, consolidated into one comment. I re-ran the mutations from my first two passes rather than reading the diff, so these are execution results.

Both second-pass findings are closed

1. Bracket-index laundering. SNEAKY: ${{ inputs['subject_name'] }} in the provenance step env, folded into --arg repo, now fails TestAttestPredicateUsesOnlyTrustedContext. The needs['validate'].outputs.* form fails too.
2. resolvedDependencies[0].uri. The second jq writer that swapped the URI for git+https://evil.example/attacker/repo@refs/heads/main now fails.

The nits check out as well. ADR-074 pairs v* tags with release.yml only, the attest.yml header carries the origin-fields scoping and the externalParameters.subjectKind carve-out, and gateTestCitePages now covers the ADR, so renaming TestAttestIsSoleSigner in ADR prose fails TestCitedGateTestsExist. ~14 is right: ten matrix legs, emit_provenance defaults true so every blob leg makes one cosign attest-blob call, and the four binary legs make a second one for the SBOM predicate.

I also re-ran every first-pass mutation as a regression check: deleting exit 1 from the builder-id guard, removing the fail-closed case arm, an org-qualified sha-pinned attest call both in publish.yml and from a newly added caller workflow, dropping every attest.yml call from release.yml, and a second jq writer making repository caller-controlled. All still caught.

The v* ruleset exists, and it does not close what I thought it closed

I created it, so that deferred housekeeping item is done. Protect release tags (v*) is active on this repository, scoped to refs/tags/v*, enforcing creation, update, deletion and non_fast_forward, with bypass limited to repository admin.

I then overstated what it buys, and I am correcting myself. It does not on its own gate the release identity:

1. attest-selftest.yml:71 gates on github.repository and nothing else. workflow_dispatch accepts a tag as its ref, so nothing holds that workflow to a branch.
2. attest.yml:192 takes CALLER_REF from github.ref, and the guard at attest.yml:286 skips its entire body when that matches refs/tags/v*. The allow_untagged: true at attest-selftest.yml:85 is never consulted and the non-production warning at attest.yml:290 never prints.
3. A run on a v* ref therefore mints the release SAN while creating no tag and moving no tag, and tag operations are all a ruleset constrains.
4. publish.yml:33-43 reasons through this same shape for itself and guards with && github.ref == 'refs/heads/main', with a comment explaining that the guard is load-bearing rather than hygiene. attest-selftest.yml has the identical dispatch shape and no equivalent.

That also makes the comment at attest-selftest.yml:33-35, that the caller ref is a branch and so the SAN carries @refs/heads/..., an assumption rather than a guarantee. None of it is introduced by this PR: attest-selftest.yml is already present at v0.2.0 and v0.2.0-rc.2 at the same blob as main. #340 has the guard.

What this means for this PR

This PR adds the sentence at ADR-074:79, SECURITY.md:84 and docs/operations/verifying-artifacts.md:106-108 saying tag protection is the control gating release identity. That sentence is not true today and becomes true once #340 lands.

So I am holding this PR until #340 merges, rather than asking you to qualify the prose and then unqualify it later. Nothing to change in those three files. Once #340 is in, the wording here is correct as written and I will approve.

Two things I would like fixed here in the meantime:

1. ADR-074:119 still draws publish.yml (tag) in the topology diagram, which contradicts the prose you corrected 40 lines above. Pre-existing line, but it is the mirror of the fix I asked for last round.
2. docs/operations/verifying-artifacts.md:115 links SECURITY.md relatively as ](../../SECURITY.md#supply-chain), while line 446 of the same file uses an absolute URL. Fern Check is green, so this is consistency rather than a build problem.

One route is still open, and it is not holding this PR

The env scan is needle-based, so it closes the two forms I demonstrated rather than the class behind them. A step added ahead of the provenance step in the same job, exporting inputs.subject_name through $GITHUB_ENV and folded into the jq call as --arg repo "${GITHUB_REPOSITORY}${SNEAKY:-}", passes test/releasepolicy, test/docspolicy and test/helm together. The scan misses it because it reads workflow, job and step env for the provenance step only. The executed assertions miss it because the harness leaves SNEAKY unset, so ${SNEAKY:-} expands empty. Running that same extracted step with SNEAKY set the way a real run would set it gives externalParameters.repository = NVIDIA/cluster-readiness-engine/../attacker-controlled. That is #339, and it needs an edit to attest.yml itself, which is the file this gate exists to defend.

Separately, I checked whether CodeRabbit's ${ID} point also applies to docs/operations/verifying-artifacts.md. It technically does, since the second ID= at line 138 satisfies the page-wide check, but I am not asking for a change: docs-verify.yml extracts and runs every block on that page against a real release, so a wrong-org pin fails there. Hardening SECURITY.md statically and leaving this page to execution is the right split.

State

CI is fully green on cf4b87c, all 13 checks including Test, UAT and Fern Check. The workflow diff against main is comment-only. The only thing blocking the merge button is review, and I am holding that for #340.

/ok to test cf4b87c

Pushed two cheap nits from the 3rd-pass hold note (docs-only; #340 is up as #341):

1. ADR-074 topology: publish.yml (tag) → publish.yml (main) so it matches the prose / on: push: branches: [main].
2. verifying-artifacts.md: SECURITY.md link now uses the same absolute GitHub URL style as the See also section (keeps #supply-chain).

Head moved to de122e6 (docs nits). Fresh trustee /ok to test needed on the current SHA for NVIDIA runners.

/ok-to-test de122e6

Fourth pass, on de122e6, now that #341 has merged as 84e994c. As before these are execution results rather than a read of the diff.

The hold is released, and both nits landed

I held this on #340 last round and said the wording here becomes correct as written once it lands. It has. The two things I asked for in the same comment are in:

1. docs/designs/074-supply-chain-attestation.md:119 now draws publish.yml (main), so the topology matches the prose 40 lines above.
2. docs/operations/verifying-artifacts.md:115 now links SECURITY.md with the same absolute GitHub URL style as line 446, #supply-chain preserved.

CI is green on de122e6, all 13 checks including Test, UAT and Fern Check, and there are no unresolved threads. #339 is still open and still not blocking this.

Two things now block the merge, and neither is visible from this PR's CI

de122e6 predates 84e994c, so nothing that ran on this branch has ever seen the two changes together. I merged origin/main into de122e6 locally to find out. Only SECURITY.md conflicts textually, and that turns out to be the smaller of the two problems.

1. test/releasepolicy does not compile after the merge

Both changes declare localAttestUses in package releasepolicy, with different types, in files git merges cleanly:

1. test/releasepolicy/attest_guards_test.go:505, landed with fix(ci): gate attest-selftest.yml to refs/heads/main (#340) #341: localAttestUses = "./.github/workflows/attest.yml", an untyped string constant used as a Uses: field in fixture structs.
2. test/releasepolicy/attest_boundary_test.go:23, from this PR: var localAttestUses = regexp.MustCompile(...), a *regexp.Regexp.

The compiler gives 13 errors with no truncation: one localAttestUses redeclared in this block, and 12 of cannot use localAttestUses (variable of type *regexp.Regexp) as string value in struct literal at attest_guards_test.go lines 622, 631, 644, 656, 668, 680, 692, 718, 725, 733, 744 and 756. The package fails to build, so every gate in it stops running, including the ones this PR adds.

Since #341 is already on main, I would rename this PR's symbol rather than the one on main. localAttestUsesPattern reads correctly against the comment above it, which already distinguishes this matcher from isAttestWorkflowCall. Three occurrences, all inside attest_boundary_test.go.

The two attest.yml edits do not interact: #341 changed the allow_untagged shell guard around line 279, this PR rewrites the header comment at lines 3 to 35. Git auto-merges them and the result is correct.

2. SECURITY.md ends up contradicting itself in adjacent paragraphs

This is the conflict that matters, and resolving it by taking either side loses something. Both changes insert at the same anchor in the same list item:

1. From fix(ci): gate attest-selftest.yml to refs/heads/main (#340) #341, on main: "Tag protection / the v* ruleset does not cover this path (no tag is created or moved)."
2. From this PR: "A same-repo uses: ./… boundary additionally rests on branch protection over .github/workflows/attest.yml for main pushes, and on tag protection / repository rulesets over v* tags for the release identity everyone pins."

The first is scoped to the pre-guard tag dispatch path and the second is a general statement, so they are reconcilable. They do not read that way six lines apart with nothing connecting them. Keep both paragraphs, put the #341 one first, and give this PR's sentence a clause acknowledging the exception so the scoping is on the page rather than in the reader's head.

Related, and nothing catches it

#341 only touched RELEASE.md and SECURITY.md on the docs side. So after this merges, docs/designs/074-supply-chain-attestation.md:79, the same file at line 163, and docs/operations/verifying-artifacts.md:107 all still say tag protection and v* rulesets are what gate the release identity, with no mention of the tags cut before the guard or of the repository-level disable of Attest Self-Test. I grepped both files: neither names v0.2.0, Attest Self-Test, the RELEASE.md rollout section, or #340. test/docspolicy passes either way, because nothing ties those pages to that residual.

I would add the qualifying clause in those three places too. That is the mirror of the fix I asked for in round three, same as the topology line was.

What I verified once both are fixed

With the rename applied and SECURITY.md resolved keeping both paragraphs:

1. go build ./... clean.
2. go vet ./test/releasepolicy/ ./test/docspolicy/ clean.
3. go test ./test/releasepolicy/ ./test/docspolicy/ ./test/helm/ all ok, releasepolicy in 51.5s.

So the substance of this PR is fine and the interaction is mechanical. The branch is also still based at #330, so it wants an update against main regardless.

Happy to approve once the rename and the SECURITY.md resolution are in.

Rebased onto latest main (includes #341 / 84e994c). Tip is now 161c07f (was de122e6). Conflicts resolved in SECURITY.md (kept #341 old-tag path + ADR-074 Build Level) and localAttestUses rename vs #341's string const. Fresh /ok to test may be needed on the new SHA.

Fifth pass, on 161c07f. Both blockers I raised last round are fixed, and I verified them on the real head rather than on a resolution of my own:

1. The rename is in as localAttestUsesRE, so attest_guards_test.go:505 keeps the string constant that came with fix(ci): gate attest-selftest.yml to refs/heads/main (#340) #341. go build ./... and go test -c -gcflags=-e ./test/releasepolicy/ are both clean, where before the merge produced 13 compile errors.
2. SECURITY.md keeps both paragraphs with the fix(ci): gate attest-selftest.yml to refs/heads/main (#340) #341 old-tag block first at lines 84 to 96 and the Build Level blocks after it at 98 to 100. The branch is now based directly on 84e994c.
3. go test ./test/releasepolicy/ ./test/docspolicy/ ./test/helm/ all pass, releasepolicy in 47.8s.

One item from that comment is still open

I also asked for a clause acknowledging the exception, and that is not in, in any of the four places.

SECURITY.md:98 still ends:

A same-repo uses: ./… boundary additionally rests on branch protection over .github/workflows/attest.yml for main pushes, and on tag protection / repository rulesets over v* tags for the release identity everyone pins.

Line 92 of the same file, six lines up, says tag protection and the v* ruleset do not cover the old-tag path. Ordering the paragraphs the way you did helps, because a reader meets the scoped statement first, but the second sentence still makes the general claim with nothing pointing back at the exception.

docs/designs/074-supply-chain-attestation.md:79, the same file at line 163, and docs/operations/verifying-artifacts.md:107 are byte-identical to de122e6, and neither file carries the exception. Being exact about what I checked, since one half of that is narrower than it first looks:

1. 074-supply-chain-attestation.md contains none of v0.2.0, Attest Self-Test, disabled_manually, #340, or RELEASE.md, at all.
2. verifying-artifacts.md does contain v0.2.0 six times and RELEASE.md once, but every v0.2.0 hit is a release-version example (TAG=v0.2.0-rc.1 at lines 28 and 136, the bundle-availability note at 419, and so on), never a pre-guard tag, and the RELEASE.md link at line 447 is the file root in See also, not #existing-tag-attest-selftest-rollout.

So the residual is documented in SECURITY.md and RELEASE.md and nowhere else, while the ADR and the verification page are the two pages someone reads to learn what gates the release identity.

To be clear about severity: the sentence is incomplete rather than false. It holds for the topology on main now that #341 is in, and test/docspolicy passes either way because nothing ties those pages to the residual. This is a docs-accuracy item, not a security defect, and I am not re-opening the hold over it.

What I would like is the same clause in those three spots, pointing at the RELEASE.md rollout section the way SECURITY.md already does. It is one sentence each. If you would rather land this as is, say so and I will file it as a follow-up against the ADR and the verification page instead.

/ok-to-test 161c07f

Rebased onto latest main (includes #334/#348/#355/#356). Tip is now 160594c (was 161c07f). Clean rebase (no conflicts). make lint 0 issues; go test ./test/releasepolicy/... and make test green locally. Fresh trustee /ok to test needed on the new SHA for NVIDIA runners.